Skip to content

Add NSM transaction test vectors#101

Merged
nuttycom merged 12 commits into
zcash:masterfrom
equilibriumco:zsf
Nov 25, 2025
Merged

Add NSM transaction test vectors#101
nuttycom merged 12 commits into
zcash:masterfrom
equilibriumco:zsf

Conversation

@tomekpiotrowski

@tomekpiotrowski tomekpiotrowski commented Aug 22, 2024 •
edited by daira
Loading

Copy link
Copy Markdown
Contributor

This adds test data generation for NSM transactions. Simply adds zip233_amount to tx id and sighash calculations.

For now NSM is developed under the ZFuture Network Update and ZFuture transaction version as the final NU and tx versions under which it'll be deployed are not decided yet.

The librustzcash PR that uses the generated data is zcash/librustzcash#1567 .

@giddie

giddie commented Oct 8, 2024

Copy link
Copy Markdown

I've updated this PR following the naming change (ZSF -> NSM).

@giddie giddie mentioned this pull request Oct 11, 2024
Merged
@tomekpiotrowski tomekpiotrowski changed the title Add ZSF transaction test vectors Add NSM transaction test vectors Dec 9, 2024
@aphelionz

aphelionz commented Apr 30, 2025

Copy link
Copy Markdown
Member

Thanks @mariopil

@aphelionz

aphelionz commented Jun 30, 2025

Copy link
Copy Markdown
Member

@str4d @daira Can this be merged?

@mariopil mariopil force-pushed the zsf branch 2 times, most recently from 07166f6 to 3cd2cbc Compare July 2, 2025 08:36
@aphelionz

aphelionz commented Jul 22, 2025

Copy link
Copy Markdown
Member

@daira or somebody - can I have permissions to run this workflow or can somebody run it?

nuttycom
nuttycom reviewed Jul 29, 2025
View reviewed changes
Comment thread zcash_test_vectors/transaction.py Outdated
nuttycom
nuttycom previously requested changes Jul 29, 2025
View reviewed changes

@nuttycom nuttycom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review not yet complete, but marking with changes requested to prevent inadvertent merge.

Comment thread zcash_test_vectors/transaction.py Outdated
daira
daira reviewed Aug 1, 2025
View reviewed changes
Comment thread zcash_test_vectors/transaction.py Outdated
daira
daira reviewed Aug 1, 2025
View reviewed changes
Comment thread zcash_test_vectors/transaction.py Outdated
daira
daira reviewed Aug 1, 2025
View reviewed changes
Comment thread zcash_test_vectors/transaction.py Outdated
daira
daira reviewed Aug 1, 2025
View reviewed changes
Comment thread zcash_test_vectors/transaction.py Outdated
daira
daira reviewed Aug 1, 2025
View reviewed changes
Comment thread zcash_test_vectors/transaction.py
daira
daira reviewed Aug 1, 2025
View reviewed changes
Comment thread zcash_test_vectors/transaction.py
daira
daira reviewed Aug 1, 2025
View reviewed changes
Comment thread zcash_test_vectors/transaction.py Outdated
daira
daira reviewed Aug 2, 2025
View reviewed changes
Comment thread zcash_test_vectors/zip_0233.py Outdated
daira
daira reviewed Aug 2, 2025
View reviewed changes
Comment thread zcash_test_vectors/zip_0244.py
daira
daira requested changes Aug 2, 2025
View reviewed changes

@daira daira left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good modulo comments.

@mariopil

mariopil commented Aug 11, 2025

Copy link
Copy Markdown
Contributor

Thank you @daira for the suggestions, I've updated the PR.

daira
daira reviewed Aug 21, 2025
View reviewed changes
Comment thread zcash_test_vectors/transaction.py

V6_TX_VERSION = 6
# TODO: change this
V6_VERSION_GROUP_ID = 0xFFFFFFFF

@daira daira Aug 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This intentionally isn't defined yet because ZIP 230 is not yet stable.

@mariopil mariopil Aug 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this to match the versions in librustzcash, ensuring the generated vectors used in the librustzcash tests are correct.

daira
daira previously approved these changes Aug 25, 2025
View reviewed changes

@daira daira left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, except that the checked-in test vectors need to be updated.

@mariopil

mariopil commented Aug 26, 2025

Copy link
Copy Markdown
Contributor

@daira I've updated zip_0244 test vectors and added zip_0233 test vectors.

@aphelionz

aphelionz commented Aug 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

How does it look now, @nuttycom?

@nuttycom

nuttycom commented Aug 26, 2025

Copy link
Copy Markdown
Contributor

How does it look now, @nuttycom?

CI will validate the generated test vectors against those that have been checked in, beyond that I'm fine with @daira's ACK.

@aphelionz

aphelionz commented Aug 26, 2025

Copy link
Copy Markdown
Member

@mariopil looks like there's a failure

@mariopil

mariopil commented Aug 26, 2025

Copy link
Copy Markdown
Contributor

@mariopil looks like there's a failure

There was a slight change in the TransactionV5 logic after @daira suggestions. It could affect the zip_0244 test vectors.

@nuttycom nuttycom self-requested a review August 27, 2025 22:15
@nuttycom nuttycom dismissed their stale review August 28, 2025 13:19

Problems have been resolved.

@aphelionz

aphelionz commented Aug 28, 2025

Copy link
Copy Markdown
Member

@mariopil @daira what are the next steps?

@daira

daira commented Aug 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

The fact that the CI is failing indicates that some of the test vectors (the "-t zcash" ones) have not been regenerated.

@mariopil

mariopil commented Sep 1, 2025

Copy link
Copy Markdown
Contributor

Thanks, I've regenerated the "zcash" vectors.

@nuttycom

nuttycom commented Sep 4, 2025

Copy link
Copy Markdown
Contributor

This looks like it now needs to be rebased on main following #109

@aphelionz

aphelionz commented Sep 12, 2025

Copy link
Copy Markdown
Member

Hi @daira may we merge this?

@daira

daira commented Oct 22, 2025

Copy link
Copy Markdown
Contributor

Sorry it's taken me a while to get to this. I will try to review it this week.

@oxarbitrage

oxarbitrage commented Oct 24, 2025

Copy link
Copy Markdown
Contributor

Sorry it's taken me a while to get to this. I will try to review it this week.

I have some (minor and optional) suggestions on this PR. It is ok to make a review or do you prefer to review as it is ?

oxarbitrage
oxarbitrage reviewed Oct 25, 2025
View reviewed changes

@oxarbitrage oxarbitrage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional suggestions and some questions/comments. All minor stuff, feel free to ignore.

Comment thread zcash_test_vectors/zip_0233.py Outdated
Comment thread zcash_test_vectors/zip_0233.py Outdated
Comment thread zcash_test_vectors/zip_0233.py Outdated
Comment thread zcash_test_vectors/zip_0233.py Outdated
Comment thread zcash_test_vectors/zip_0244.py
Comment thread zcash_test_vectors/transaction.py

return ret

class TransactionV6(TransactionV5):

@oxarbitrage oxarbitrage Oct 25, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: In #108, the TransactionV5 is renamed to TransactionBase:

zcash-test-vectors/zcash_test_vectors/transaction.py

Line 420 in 3e6f9eb

class TransactionBase(object):

Then TransactionV5(TransactionBase) and TransactionV6(TransactionBase) are defined. I think that makes sense.

Given that this PR is probably going to be merged first, should we apply that change here ?

daira
daira reviewed Nov 25, 2025
View reviewed changes

@daira daira left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

daira
daira approved these changes Nov 25, 2025
View reviewed changes

@daira daira left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@nuttycom nuttycom merged commit d8dd009 into zcash:master Nov 25, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oxarbitrage oxarbitrage oxarbitrage left review comments

@daira daira daira approved these changes

@nuttycom nuttycom Awaiting requested review from nuttycom

+1 more reviewer

@mariopil mariopil mariopil left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tomekpiotrowski @giddie @aphelionz @mariopil @nuttycom @daira @oxarbitrage